-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ensure consistent dihedral angle computations with OpenMM and mdanalysis #1247
base: master
Are you sure you want to change the base?
Conversation
@badisa when you get a chance, not sure why this test is failing:
|
This test has been flaky in the past, will play around with it. Easy answer is to change the asertion to |
rkj[d] = vkj; | ||
rkl[d] = vkl; | ||
rkj_norm_square += vkj * vkj; | ||
r0[d] = coords[i_idx * D + d] - coords[j_idx * D + d]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While here, might be good to use the same terminology for the reference and the kernel. Here you use i, j, k, l and in the reference you use xa, xb, xc, xd
rkj = cj - ck | ||
rkl = cl - ck | ||
# (ytz): Feb 4th 2024, switch to OpenMM convention | ||
# https://github.com/openmm/openmm/blob/master/platforms/reference/src/SimTKReference/ReferenceProperDihedralBond.cpp#L97C23-L97C32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Be good to have a permalink in case openmm changes this code ever
Sigh - the phase is defined such that theres a local maxima at theta=phase. If we want to define a minima, we'd need to offset with new_phase=pi + old_phase |
This PR addresses an issue where we compute the dihedral angle in a way that's the negative of what OpenMM and mdanalysis computes. Note that this does not affect energy/force computations for currently used forcefields (both ligand and protein), since they explicitly define phases of 0 and pi, resulting in a symmetric energy profile. This is mostly a preventative measure for the future if we do implement non-symmetric periodic torsions (either via restraints or using a new forcefield).